Skip to content

fix: only show notification when renotify is enabled in main process#3302

Open
prath47 wants to merge 1 commit into
RocketChat:masterfrom
prath47:fix-3301
Open

fix: only show notification when renotify is enabled in main process#3302
prath47 wants to merge 1 commit into
RocketChat:masterfrom
prath47:fix-3301

Conversation

@prath47
Copy link
Copy Markdown

@prath47 prath47 commented Apr 15, 2026

fix #3301

From previous commit i can see there is this variable "_renotify" got removed
just re added that
this should fix the issue

Tests:
Lint
Manual Test of fix

Summary by CodeRabbit

  • Bug Fixes
    • Improved notification behavior to only re-display when explicitly requested, preventing unnecessary re-triggers during routine updates.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 15, 2026

CLA assistant check
All committers have signed the CLA.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e8c840c2-6216-4251-8e03-a1c68a0c32c5

📥 Commits

Reviewing files that changed from the base of the PR and between d865454 and 7af3305.

📒 Files selected for processing (1)
  • src/notifications/main.ts
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: Use TypeScript for all new code in this codebase unless explicitly told otherwise
Use Fuselage components from @rocket.chat/fuselage for all UI work — only create custom components when Fuselage doesn't provide the needed functionality
Check Theme.d.ts for valid color tokens when working with Fuselage components
Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
TypeScript code must use strict mode
Use React functional components with hooks instead of class components
Redux actions must follow the FSA (Flux Standard Action) pattern
Use camelCase for file naming
Use PascalCase for component file names (React components)
Write self-documenting code through clear naming — avoid unnecessary comments

Files:

  • src/notifications/main.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: Ram-sah19
Repo: RocketChat/Rocket.Chat.Electron PR: 3254
File: .github/workflows/build-release.yml:80-94
Timestamp: 2026-03-11T06:38:40.426Z
Learning: In the RocketChat/Rocket.Chat.Electron repository, the issues flagged in `.github/workflows/build-release.yml` (e.g., `node12` runtime in the release action and missing `snapcraft_token` input), i18n files, and `electron-builder.json` are pre-existing in the `develop` branch and are pulled in during merge conflict resolution. Do not flag these as new issues introduced by PRs that only modify `src/injected.ts` and `src/ui/main/rootWindow.ts`.
🔇 Additional comments (1)
src/notifications/main.ts (1)

207-209: Conditional show() on renotify is the right fix.

This change prevents unnecessary re-showing on updates and matches the intended “notify again only when renotify is enabled” behavior for tagged notifications.


Walkthrough

The updateNotification function in src/notifications/main.ts now conditionally calls notification.show() only when the _renotify flag is truthy, preventing unnecessary re-triggering of notifications on every update.

Changes

Cohort / File(s) Summary
Notification Update Conditional
src/notifications/main.ts
Modified updateNotification to call notification.show() only when _renotify is truthy, preventing duplicate notification triggers while preserving all other update logic.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested labels

type: bug

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: conditionally showing notifications only when renotify is enabled in the main process.
Linked Issues check ✅ Passed The code change directly addresses the linked issue #3301 by reintroducing the _renotify flag check to prevent multiple notifications per message.
Out of Scope Changes check ✅ Passed All changes are scoped to the notification display logic in src/notifications/main.ts and directly relate to fixing the duplicate notifications issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple Notifications for the same message

2 participants